Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing deprecated call to Configuration::setSchemaAssetsFilter() + extendable #935

Merged
merged 1 commit into from Apr 6, 2019

Conversation

weaverryan
Copy link
Contributor

@weaverryan weaverryan commented Mar 20, 2019

Hi!

If you're using doctrine/dbal 2.9 or higher, this removes the deprecated call to Configuration:: setFilterSchemaAssetsExpression(). It also makes the "schema filter" extendable via a tag. The use-case is when a 3rd-party bundle manages a table automatically for you through DBAL and you don't want, for example, Doctrine migrations (https://github.com/doctrine/migrations/blob/cf7f94b0bcfbd7d9c240aee759dc6619bc9ad796/lib/Doctrine/Migrations/Generator/DiffGenerator.php#L117) / doctrine:schema:update to suggest removing that table because it's not found in the mapping metadata.

One real-world use-case will be for #868: when the Doctrine transport is added to Messenger, a table will be created for storing messages (either automatically by Messenger or manually by the user - their choice). DoctrineBundle itself could use this hook to avoid migrations trying to remove that table once it's created.

Thanks!

Dbal/RegexSchemaAssetFilter.php Outdated Show resolved Hide resolved
DependencyInjection/Compiler/DbalSchemaFilterPass.php Outdated Show resolved Hide resolved
$connectionFilters = [];
foreach ($filters as $id => $tagAttributes) {
foreach ($tagAttributes as $attributes) {
$name = isset($attributes['connection']) ? $attributes['connection'] : $container->getParameter('doctrine.default_connection');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should ignoring the name apply it to the default connection, or to all connections ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering about this too. Making it apply to all connections is actually more useful - as, for example, Symfony's session system could (in theory) say: "Hey, ignore a session table that exits in any connection... I'm not sure which connection is used for the PDO instance I'm using".

But, that also seems like a possible, legitimate edge-case: I want to ignore session on connection 1, but I really DO have a session table on connection2 that I'm managing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Ryan here: users should know which connection handles which tables and can build their ignore lists accordingly.

Dbal/SchemaAssetFilterInterface.php Outdated Show resolved Hide resolved
@weaverryan
Copy link
Contributor Author

Feedback handled!

@alcaeus alcaeus added this to the 1.11.0 milestone Apr 1, 2019
@alcaeus alcaeus self-requested a review April 1, 2019 05:42
Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've refrained from commenting on code style since I'll only be adding doctrine/coding-standard in a future PR. There are a couple of changes I'd suggest making.

Dbal/RegexSchemaAssetFilter.php Show resolved Hide resolved
@weaverryan
Copy link
Contributor Author

Updated and rebased! Thanks for checking it!

@alcaeus
Copy link
Member

alcaeus commented Apr 2, 2019

Some tests still seem to fail: https://travis-ci.org/doctrine/DoctrineBundle/jobs/514116227#L347

  1. Doctrine\Bundle\DoctrineBundle\Tests\DependencyInjection\XmlDoctrineExtensionTest::testDbalSchemaFilter
    Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "doctrine.dbal.default_connection.configuration".
    /home/travis/build/doctrine/DoctrineBundle/vendor/symfony/dependency-injection/ContainerBuilder.php:1019
    /home/travis/build/doctrine/DoctrineBundle/Tests/DependencyInjection/AbstractDoctrineExtensionTest.php:773
  2. Doctrine\Bundle\DoctrineBundle\Tests\DependencyInjection\YamlDoctrineExtensionTest::testDbalSchemaFilter
    Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "doctrine.dbal.default_connection.configuration".
    /home/travis/build/doctrine/DoctrineBundle/vendor/symfony/dependency-injection/ContainerBuilder.php:1019
    /home/travis/build/doctrine/DoctrineBundle/Tests/DependencyInjection/AbstractDoctrineExtensionTest.php:773

@weaverryan
Copy link
Contributor Author

Sorry about that! It's better now - only php nightly failing for unrelated reasons.

@alcaeus alcaeus force-pushed the dbal_filter_plugin branch 2 times, most recently from 80a93e7 to 38d32c2 Compare April 4, 2019 09:48
…ssion()

... and making the schema filter stuff a "plugin" system, which is the
intention of the new way it's handled in doctrine/dbal
@alcaeus
Copy link
Member

alcaeus commented Apr 4, 2019

I've rebased the branch on top of master to fix build the nightly build issue and fixed CS. Thanks for building this @weaverryan!

@alcaeus alcaeus self-assigned this Apr 4, 2019
@alcaeus alcaeus merged commit eb88c42 into doctrine:master Apr 6, 2019
@alcaeus
Copy link
Member

alcaeus commented Apr 6, 2019

Thanks @weaverryan!

@alcaeus alcaeus added the ⭐️ EUFOSSA Hackathon https://symfony.com/blog/the-symfony-and-api-platform-hackathon-is-coming label Apr 7, 2019
@weaverryan weaverryan deleted the dbal_filter_plugin branch April 8, 2019 13:34
@bendavies bendavies mentioned this pull request May 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⭐️ EUFOSSA Hackathon https://symfony.com/blog/the-symfony-and-api-platform-hackathon-is-coming Improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants